-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Next major version: V5 #1504
Next major version: V5 #1504
Conversation
50a4525
to
f4a4c03
Compare
@luin any chance we can squeeze in resp3 (dont remember this being supported, but might be wrong) / client side caching here? glad to help! |
and another idea is native opentelemetry instrumentation |
It hasn't been supported 🤣 . For myself, I don't feel that resp3 will bring a lot of real value, and I'm not sure as a driver, what kind of API needs to be provided to support the new features that resp3 brings. Similarly, client side caching does not seem to be as meaningful for a language that uses GC. But I haven't thought much about this, so open to suggestions! If we want to support resp3, v5 seems to be a good chance to do so.
Can you elaborate a little bit more on this? Does it mean to provide an interface to get the average command execution time or something similar? |
it sort of allows you to perform diagnostics better and get a complete trace of a request when every part of your request chain is instrumented, typically you'd use async_hooks, but can be anything https://github.com/elastic/apm-agent-nodejs/blob/main/lib/instrumentation/modules/ioredis.js is an example of an existing instrumentation, which can be bridged to opentracing/opentelementry, fed to something like https://github.com/jaegertracing/jaeger for visualization/search |
I don;t think GC is relevant in terms of client-side caching here, in my opinion main benefit here is lower latency to access information, along with guarantees that information is still what server has. https://redis.io/topics/client-side-caching |
Resp3 changes response formats and we should mention that. eg. booleans are converted to true/false instead of 1/0: https://redis.io/topics/lua-api#lua-to-resp3-type-conversion Therefore I'd schedule resp3 with another version later or support opt-out (maybe opt-in using |
BREAKING CHANGE: 1. We now require Node.js v10.12.0 or newer. 2. We now only work with Redis v3.0.0 or newer. 3. `Redis` can't be called as a function anymore as it's now a class. Please change `Redis()` to `new Redis()`. Note that `Redis()` was already deprecated in the previous version.
0db3906
to
8649114
Compare
} else if (!this.stream.writable) { | ||
writable = false; | ||
// @ts-expect-error | ||
} else if (this.stream._writableState && this.stream._writableState.ended) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still needed with node 10? 😉
// TODO WeakMap | ||
// @ts-expect-error | ||
command.__is_reject_overwritten = true; | ||
if (!node && !REJECT_OVERWRITTEN_COMMANDS.has(command)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep in mind that the command.promise
& command.reject
could get overwritten in some cases.
But at the moment it's only done in the pipeline and for that node
should be present here.
It's probably good enough at the moment, but .has(command.promise)
might be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Will think about it!
BREAKING CHANGE: `slotsRefreshInterval` is disabled by default, previously, the default value was 5000.
Renamed to the main branch. Published v5.0.0-beta.1. |
Yeah I think that makes sense. Let's delay this to the next major release. |
Hey @marcbachmann @TysonAndre 👋 , thanks for the contributions to ioredis. I've just invited you to become collaborators on this repository. Feel free to help review new pull requests in the future if you have time (and it doesn't matter at all if you don't have time 😄). Appreciate it 🙏. You can refer to https://github.com/luin/ioredis/blob/main/.github/CONTRIBUTING.md for some notes. |
Hey @shaharmor , The option at some level introduces side effects as it consumes resources, especially during a failover, that by default every 5 seconds all nodes may have to be recreated (if |
Closes #1292
Major changes:
NOSCRIPT
in pipeline mode. Pipeline-based script loading and retries #1499info
on readyCheck topersistence
section #1293